Skip to content
This repository has been archived by the owner on Apr 27, 2021. It is now read-only.

enable setting upstreams dynamically via HTTP endpoint #16

Closed
wants to merge 34 commits into from
Closed

enable setting upstreams dynamically via HTTP endpoint #16

wants to merge 34 commits into from

Conversation

ElvinEfendi
Copy link

@ElvinEfendi ElvinEfendi commented Mar 2, 2018 •
edited
Loading

The PR exposes an HTTP endpoint for configuring upstream peers, and changes controller code so that instead of reloading it POSTs the list of new upstream peers(endpoints) into Nginx.

For any change that is not an Endpoint change, it falls back to reload behaviour so the change has to be strictly Endpoint change so that it skips reloading.

Also note that skipping reload does not mean skipping updating the config file - the config file on disk gets updated as usual - this is important for resiliency.

Next steps:

  • Consider using https://github.com/openresty/lua-resty-balancer or design and implement a better load balancing interface and at least Nginx's default LB algorithms
  • Make LB load balancing algorithm configuration per backend, and add a new field to ingress.Backend that represents which LB algorithm should be used for that backend endpoints.
  • Polish the code, add tests
  • Using certificate_by_lua make certificate/secret updates dynamic as well

@Shopify/edgescale

@aledbf
Copy link

aledbf commented Mar 3, 2018

@ElvinEfendi have you seen kubernetes#2152 ?
That PR provides basically the same thing you are trying to achieve here.

@aledbf
Copy link

aledbf commented Mar 3, 2018 •
edited
Loading

@ElvinEfendi are you interested in collaborating in that PR?

Edit: Today I will post a temporal docker image with the content of the PR

@ElvinEfendi
Copy link
Author

@aledbf thanks for the link, I did not know about it. Yeah the end goal seems to be the same, but that PR is using a different approach. I'm almost done with the proof of concept. Will polish it a bit more then make a PR upstream where we can discuss further.

are you interested in collaborating in that PR?

I'm definitely interested, how do you see us proceed?

@aledbf
Copy link

aledbf commented Mar 3, 2018

Will polish it a bit more then make a PR upstream where we can discuss further.

I would prefer to continue in the PR already opened. Can we do that?
Maybe you can add your comments/suggestions/complains in the PR?
I really want to avoid forks with the same feature but with different implementations

ElvinEfendi
ElvinEfendi commented Mar 3, 2018
View reviewed changes
internal/ingress/controller/controller.go Outdated
@@ -167,11 +170,79 @@ func (n *NGINXController) syncIngress(item interface{}) error {
if !n.isForceReload() && n.runningConfig.Equal(&pcfg) {
glog.V(3).Infof("skipping backend reload (no changes detected)")
return nil
} else if !n.isForceReload() && len(n.runningConfig.Backends) == len(pcfg.Backends) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zrdaley this is how I ended up implementing it. The idea is to detect only Endpoints changes in case of any other change we fall back to reloading.

@ElvinEfendi ElvinEfendi force-pushed the dynamic-upstreams branch 2 times, most recently from f6188b6 to 98a408d Compare March 3, 2018 07:15
@ElvinEfendi
Copy link
Author

ok, I have the first per namespace/service working load balancing

2018/03/04 05:18:23 [info] 30#30: *293 [lua] configuration.lua:36: update_backend(): updating backend: another-test-an-app1-80, context: ngx.timer
2018/03/04 05:18:23 [info] 29#29: *294 [lua] configuration.lua:36: update_backend(): updating backend: another-test-an-app1-80, context: ngx.timer
I0304 05:18:25.371379       5 controller.go:190] only endpoints have changed, skipping reload and posting them to Nginx via HTTP request
I0304 05:18:25.371446       5 controller.go:233] upstream-default-backend is same as running config. number of endpoints:
I0304 05:18:25.371471       5 controller.go:233] elvin-test-an-app1-80 is same as running config. number of endpoints:
2018/03/04 05:18:25 [info] 30#30: *297 [lua] configuration.lua:108: f(): backend data was updated for another-test-an-app1-80: {"name":"another-test-an-app1-80","service":{"metadata":{"name":"an-app1","namespace":"another-test","selfLink":"/api/v1/namespaces/another-test/services/an-app1","uid":"c3950ded-1f63-11e8-958f-fe295f9f64c3","resourceVersion":"92563","creationTimestamp":"2018-03-04T04:23:17Z","labels":{"app":"default-http-backend"},"annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"v1\",\"kind\":\"Service\",\"metadata\":{\"annotations\":{},\"labels\":{\"app\":\"default-http-backend\"},\"name\":\"an-app1\",\"namespace\":\"another-test\"},\"spec\":{\"ports\":[{\"port\":80,\"targetPort\":8080}],\"selector\":{\"app\":\"default-http-backend\"}}}\n"}},"spec":{"ports":[{"protocol":"TCP","port":80,"targetPort":8080}],"selector":{"app":"default-http-backend"},"clusterIP":"10.108.103.16","type":"ClusterIP","sessionAffinity":"None"},"status":{"loadBalancer":{}}},"port":80,"secure":false,"secureCACert":{"secret":"","caFilename":"","pemSha":""},"sslPassthrough":false,"endpoints":[{"address":"172.17.0.11","port":"8080","maxFails":0,"failTimeout":0,"target":{"kind":"Pod","namespace":"another-test","name":"default-http-backend-55c6c69b88-zhsxm","uid":"c3907f89-1f63-11e8-958f-fe295f9f64c3","resourceVersion":"92581"}},{"address":"172.17.0.12","port":"8080","maxFails":0,"failTimeout":0,"target":{"kind":"Pod","namespace":"another-test","name":"default-http-backend-55c6c69b88-4p8tl","uid":"b5701772-1f64-11e8-958f-fe295f9f64c3","resourceVersion":"92902"}}],"sessionAffinityConfig":{"name":"","cookieSessionAffinity":{"name":"","hash":""}}}, client: 127.0.0.1, server: , request: "POST /configuration/backends/another-test-an-app1-80 HTTP/1.1", host: "localhost:18080"
I0304 05:18:25.373146       5 controller.go:230] endpoints for another-test-an-app1-80 have been updated
127.0.0.1 - [127.0.0.1] - - [04/Mar/2018:05:18:25 +0000] "POST /configuration/backends/another-test-an-app1-80 HTTP/1.1" 201 5 "-" "Go-http-client/1.1" 1755 0.000 [-] - - - -
2018/03/04 05:18:25 [info] 30#30: *297 client 127.0.0.1 closed keepalive connection
2018/03/04 05:18:25 [info] 30#30: *298 [lua] configuration.lua:36: update_backend(): updating backend: another-test-an-app1-80, context: ngx.timer
2018/03/04 05:18:25 [info] 29#29: *299 [lua] configuration.lua:36: update_backend(): updating backend: another-test-an-app1-80, context: ngx.timer

ElvinEfendi
ElvinEfendi commented Mar 4, 2018
View reviewed changes
internal/ingress/controller/template/template.go Outdated
@@ -313,7 +313,8 @@ func buildProxyPass(host string, b interface{}, loc interface{}) string {
}

// defProxyPass returns the default proxy_pass, just the name of the upstream
defProxyPass := fmt.Sprintf("proxy_pass %s://%s;", proto, upstreamName)
//defProxyPass := fmt.Sprintf("proxy_pass %s://%s;", proto, upstreamName)
defProxyPass := fmt.Sprintf("proxy_pass %s://upstream_balancer;", proto)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be done only if the dynamic reconfiguration is enabled for the vhost/ingress

@ElvinEfendi ElvinEfendi mentioned this pull request Mar 4, 2018
Closed
@ElvinEfendi
Copy link
Author

I'm also adding support for configuring load balancing algorithm per ingress at kubernetes#2167.

@ElvinEfendi
Copy link
Author

ElvinEfendi commented Mar 5, 2018 •
edited
Loading

This is now running in our staging environment. The image is public at index.docker.io/elvinefendi/nginx-ingress-controller:0.0.1

To enable the dynamic configuration one needs to start the controller with --enable-dynamic-configuration flag. If you wanna see more Lua logs you can also set error log level to "info" to see how the balancer chooses different upstream peers and a bit more.

The next steps are

  1. Add tests at least for the new Go code
  2. Implement EWMA algorithm

@aledbf as you suggested I'm holding off of making upstream PR and also gave my feedback on the other PR you referenced. My priority with this PR will be to make it more stable and then implement EWMA balancing algorithm. And I'm also going to provide an extendable interface for anyone who wants to contribute with a different LB algorithm.

If you wanna try this the image is linked above.

The general workflow

Controller side:

  • when force reload is set, we unconditionally POST to Lua endpoint after reload
  • when controller gets a regular update without force reload, we first check if the change is only about backends
  • if yes then POST the list of new backends to the Lua endpoint
  • if successfully POSTed then regenerating new config and save it but skip reload
  • if not then fallback to reloading

Lua side:

  • configuration.lua: this middleware accepts JSON payload and stores it in the respective shared Lua dictionary
  • balancer.lua: this middleware periodically runs a function per worker that decodes raw config that's stored in the shared Lua dictionary and stores it in local per Nginx worker cache. It also takes care of resetting respective LB state. The second responsibility of this middleware is to actually call the respective load balancing algorithm for the upstream and set current upstream peer.

@ElvinEfendi ElvinEfendi mentioned this pull request Mar 6, 2018
Merged
@ElvinEfendi
Copy link
Author

The change has been merged into master at #23 and upstream PR is at kubernetes#2174.

No need for this PR anymore.

@ElvinEfendi ElvinEfendi closed this Mar 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ElvinEfendi @aledbf